-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename abortWriting()/writingAborted → reset()/remoteCanceled & abortReading()/readingAborted → cancel()/remoteReset #248
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
abortReading()
is being removed because it is considered a duplicate of cancel(reason)
, correct?
With readingAborted
removed, is the application to know that a RST_STREAM
has been received via reader.closed
? Does this provide an errorCode
value?
line 796/797: I don't find "drop/dropped" to be more informative than "abortWriting/writingAborted".
line 810: HTTP/2 fallback would not involve a STOP_SENDING
frame. So being specific about Http3Transport would be better.
Line 828: HTTP/2 fallback would not involve a RST_STREAM
frame. So being specific about Http3Transport would be better.
line 974, 990, 994, 1002: can you add the code argument?
@domenic do you know if the |
It is sent to the underlying source, which can interpret it as desired. |
Reg: |
Rebased |
Good idea! I've opened #252 on it. In the meantime I've updated this PR to s/readingAborted/remoteReset/ as well (we can reuse the same name on readable and writable now, since we got rid of mixins). This seems like a reasonable incremental step, so I'm hoping we can merge this PR and treat #252 as a separate idea. PTAL |
index.bs
Outdated
1. Remove [=this=] from the transport's [=[[OutgoingStreams]]=]. | ||
1. Let |reason| be |abortInfo|.errorCode. | ||
1. If it hasn't started already, start the closing procedure by sending a | ||
RST_STREAM frame with its error code set to the value of |reason|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit; s/RST_STREAM/RESET_STREAM/
index.bs
Outdated
|reason|. | ||
|
||
The {{SendStream}}'s <dfn>abortAlgorithm</dfn> (TODO) is: | ||
1. Let |stream| be the {{ReceiveStream}} object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit; SendStream
Hmm, I'm confused (again). Sending a
OR, as commented above, we can remove
|
I think so (my fault). Sending a |
@aboba no it doesn't. Good observation. So I've reinstated |
Meeting:
|
Co-authored-by: Martin Thomson <[email protected]>
Not quite. In general you should use the algorithms from https://streams.spec.whatwg.org/#other-specs , so in this case https://streams.spec.whatwg.org/#readablestream-error and ... oops, I didn't create such a wrapper for WritableStream, let me do so right now. (It will be a wrapper around WritableStreamDefaultControllerError though, not StartErroring. |
@yutakahirano does it look good now? |
Still LGTM. Let's land this change and continue the discussion (at #260 for example). |
I'll take this as a ✅ and merge this. |
Fixes #242.
Preview | Diff